Skip to content

More cleanup#5

Merged
rahil-c merged 1 commit intorahil-c:scan-plan-model-parser-onlyfrom
amogh-jahagirdar:scan-plan-model-parser-only-cleanup
Oct 31, 2024
Merged

More cleanup#5
rahil-c merged 1 commit intorahil-c:scan-plan-model-parser-onlyfrom
amogh-jahagirdar:scan-plan-model-parser-only-cleanup

Conversation

@amogh-jahagirdar
Copy link
Collaborator

@amogh-jahagirdar amogh-jahagirdar commented Oct 22, 2024

No description provided.

@github-actions github-actions bot added the CORE label Oct 22, 2024
@rahil-c
Copy link
Owner

rahil-c commented Oct 22, 2024 •
edited
Loading

@amogh-jahagirdar I think some tests are failing otherwise change looks fine to me. Seems to be failing at the parser tests because i think we now are using the fileScanTask.spec() which wont work as is i believe because at the time its unbound file scan tasks in the test. I think either we have to make sure they are bound so this error does not surface.

TestFetchPlanningResultResponseParser > roundTripSerdeWithValidStatusAndFileScanTasks() FAILED
    java.lang.UnsupportedOperationException: spec() is not supported in UnboundBaseFileScanTask
        at org.apache.iceberg.UnboundBaseFileScanTask.spec(UnboundBaseFileScanTask.java:44)
        at org.apache.iceberg.rest.responses.TableScanResponseParser.serializeScanTasks(TableScanResponseParser.java:109)
        at org.apache.iceberg.rest.responses.FetchPlanningResultResponseParser.toJson(FetchPlanningResultResponseParser.java:55)
        at org.apache.iceberg.rest.responses.FetchPlanningResultResponseParser.lambda$toJson$0(FetchPlanningResultResponseParser.java:42)
        at org.apache.iceberg.util.JsonUtil.generate(JsonUtil.java:75)
        at org.apache.iceberg.rest.responses.FetchPlanningResultResponseParser.toJson(FetchPlanningResultResponseParser.java:42)
        at org.apache.iceberg.rest.responses.TestFetchPlanningResultResponseParser.roundTripSerdeWithValidStatusAndFileScanTasks(TestFetchPlanningResultResponseParser.java:203)

@amogh-jahagirdar amogh-jahagirdar force-pushed the scan-plan-model-parser-only-cleanup branch 2 times, most recently from 59ccfc8 to 2f69389 Compare October 31, 2024 04:21
rahil-c
rahil-c reviewed Oct 31, 2024
View reviewed changes
core/src/main/java/org/apache/iceberg/rest/responses/TableScanResponseParser.java

PartitionSpec partitionSpec = specsById.get(fileScanTask.file().specId());
RESTFileScanTaskParser.toJson(fileScanTask, deleteFileReferences, partitionSpec, gen);
PartitionSpec spec = specsById.get(fileScanTask.file().specId());
Copy link
Owner

@rahil-c rahil-c Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there also be validation on the map itself specsById to ensure this is not null otherwise this will throw npe?

Copy link
Collaborator Author

@amogh-jahagirdar amogh-jahagirdar Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it's good to add validation for that, but not here imo, I think I'd add it a higher level for each response so that it's clear in the failure that a specific response requires the specs by ID to be set when serializing

@amogh-jahagirdar amogh-jahagirdar force-pushed the scan-plan-model-parser-only-cleanup branch from 2f69389 to 7544d4c Compare October 31, 2024 18:51
@rahil-c rahil-c merged commit c68661f into rahil-c:scan-plan-model-parser-only Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rahil-c rahil-c rahil-c approved these changes

Assignees

No one assigned

Labels

CORE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@amogh-jahagirdar @rahil-c